Update MobileModel parseDescriptor to support iPhone 11 #3277
Conversation
It was parsing only the first digit of the version and using that in a comparison check to determine if the version is greater than 5. This meant for the iPhone 11 it was comparing 1 > 5, returning an incorrect result. It now supports multi-digit version numbers (i.e. 11), including support for a suffix in the version (i.e. 11S), just in case...
| versionString = versionString.substring(0, 1); | ||
| } else if (versionString.matches("\\d{2}[^\\d]")) { | ||
| versionString = versionString.substring(0, 2); | ||
| } | ||
| try { | ||
| int version = Integer.parseInt(versionString); | ||
| return version > 5; |
There was a problem hiding this comment.
One thing I would like clarification on, if anyone knows...
From the comment on line 134:
// iPhone 6 does not support isContentAvailable, iPhone 7 does. // We don't know for other versions, but lets assume all above iPhone 6 are ok.
If that is the case, I assume this should be changed to:
return version > 6;
Unless the comment is incorrect?
I am not familiar with isContentAvailable, but is this capability really device dependent?
It appears content-available was added to notifications in iOS7: https://stackoverflow.com/questions/20690220/proper-use-of-content-available-in-ios-7-push-notifications
And iOS7 is supported as far back as iPhone 4: http://osxdaily.com/2013/06/10/ios-7-supported-devices/
So then maybe it should be:
return version >= 4;
There was a problem hiding this comment.
Yes, it looks like as if the check is wrong. Even if the hardware string is used it must be even higher (iPhone9,1+ == iPhone 7). Maybe @joachimneumann could shed some light on this.
There was a problem hiding this comment.
Hi,
you are probably right. I have copied the file MobileModel.java from somewhere on the Internet and also, I have not spend a lot of time to identify the hardware and software requirements. I only remember that the notifications did not work on an iPhone 6, but they did work on an iPhone 6S. Since newer operating systems (still true for iOS 13) run on the iPhone 6S, but not on the iPhone 6 and I assumed that most user will update their OS, I opted for the lazy route.
If you have older hardware and/or iPhones with older OS versions to test this, feel free to correct the requirements.
Joachim
There was a problem hiding this comment.
Thanks! I have updated this PR to reflect isContentAvailable supports iPhone 6s and newer.
This is just something I stumbled upon during a review.
It was parsing only the first digit of the version and using that in a
comparison check to determine if the version is greater than 5.
This meant for the iPhone 11 it was comparing 1 > 5, returning an
incorrect result.
It now supports multi-digit version numbers (i.e. 11), including
support for a suffix in the version (i.e. 11S), just in case...